Skip to content

Conversation

@incertum
Copy link
Contributor

Deprecates all metric creation methods that accept loose string parameters for name and help text.

This standardizes metric creation on the MetricNameDescriptor type, which requires a metricName, unitName and helpText for all metrics. This improves compliance with official Prometheus client library guidelines ("A MetricFamily MUST have a name, HELP, TYPE, and UNIT metadata.").

The deprecated methods will be removed in a future major version (see #145).

CC @ktoso, thank you!

@incertum incertum added the 🆕 semver/minor Adds new public API. label Oct 14, 2025
@incertum incertum force-pushed the deprecation/help-unit-required-future branch from ac36981 to 0babf92 Compare October 14, 2025 19:42
@incertum
Copy link
Contributor Author

Additional note: We should avoid redirecting to the public APIs that have now been marked as deprecated. I wanted to first ask for your opinion on this @ktoso . Happy to make these additional changes.

@incertum
Copy link
Contributor Author

Additional note: We should avoid redirecting to the public APIs that have now been marked as deprecated. I wanted to first ask for your opinion on this @ktoso . Happy to make these additional changes.

I've pushed a suggestion.

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, AFAICS this keeps source compat

@incertum incertum enabled auto-merge October 21, 2025 16:52
@incertum incertum disabled auto-merge October 21, 2025 16:53
Deprecates all metric creation methods that accept loose string parameters for name and help text.

This standardizes metric creation on the MetricNameDescriptor type, which requires a metricName, unitName and helpText for all metrics. This improves compliance with official Prometheus client library guidelines ("A MetricFamily MUST have a name, HELP, TYPE, and UNIT metadata.").

The deprecated methods will be removed in a future major version (see swift-server#145).

Signed-off-by: Melissa Kilby <[email protected]>
@incertum incertum force-pushed the deprecation/help-unit-required-future branch 2 times, most recently from 5e3e2d5 to c7bbfdd Compare October 27, 2025 21:46
@incertum incertum force-pushed the deprecation/help-unit-required-future branch from c7bbfdd to f1c267b Compare October 27, 2025 21:54
@incertum
Copy link
Contributor Author

Please re re-review the latest minor fixes re CI failures, thanks! @FranzBusch @ktoso

@FranzBusch
Copy link
Contributor

Can we avoid removing the warnings as errors setting? Are the deprecation warnings in the tests? If so you an mark the tests as deprecated which will silence those warnings.

@incertum incertum force-pushed the deprecation/help-unit-required-future branch from f1c267b to e48116e Compare October 28, 2025 22:44
@incertum
Copy link
Contributor Author

Can we avoid removing the warnings as errors setting? Are the deprecation warnings in the tests? If so you an mark the tests as deprecated which will silence those warnings.

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

Mark tests as deprecated (for now). They will be refactored in the future.

Co-authored-by: Franz Busch <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
@incertum incertum force-pushed the deprecation/help-unit-required-future branch from e48116e to f70db0f Compare October 28, 2025 22:54
@FranzBusch
Copy link
Contributor

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

It looks like only 5.10 and 6.0 are producing these warnings. Can we instead disable warnings-as-errors on just those versions. FWIW, we should also drop support for 5.10.

@incertum incertum force-pushed the deprecation/help-unit-required-future branch from ee9de4a to e0dc338 Compare October 29, 2025 15:58
@incertum incertum force-pushed the deprecation/help-unit-required-future branch from e0dc338 to d53cfd7 Compare October 29, 2025 16:00
@incertum
Copy link
Contributor Author

While it silenced it locally swift test -Xswiftc -warnings-as-errors 2 CI tests are now again erroring out.

It looks like only 5.10 and 6.0 are producing these warnings. Can we instead disable warnings-as-errors on just those versions. FWIW, we should also drop support for 5.10.

I have updated the override matrix (it wasn't yet configured for 6.0 and 6.1). It turns out that while having -Xswiftc -warnings-as-errors works on a macOS it appears just not to work the "same way" on these Linux runners.

I would propose to defer this and unblock this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants